scripts/download_pixlet.sh (1)

117-120: Avoid ls | grep when listing binaries.

This is fragile with non-standard filenames. Prefer a glob-based listing.

♻️ Proposed refactor

-echo "Installed binaries:"
-ls -lh "$BIN_DIR" | grep -v "^total" || echo "No binaries found"
+echo "Installed binaries:"
+shopt -s nullglob
+files=("$BIN_DIR"/*)
+if ((${`#files`[@]})); then
+    ls -lh "${files[@]}"
+else
+    echo "No binaries found"
+fi

starlark/SCALING_GUIDE.md (1)

17-22: Consider documenting auto-scaling mode (magnify=0).

The PR summary mentions that magnify=0 enables auto-scaling mode which "calculates optimal magnification for display dimensions." However, this guide only documents explicit magnify values (1-4). Consider adding documentation for the auto mode:

{
  "magnify": 0  // Auto-detect optimal magnification based on display size
}

This would help users understand when to use auto vs. explicit magnification.

starlark/starlarkplan.md (1)

1-2: Minor: Remove remnant title.

Line 1 contains "Starlark plan.txt" which appears to be a leftover filename that should be removed since the actual title is on line 2.

-Starlark plan.txt
 # Plan: Tidbyt/Tronbyt .star App Integration for LEDMatrix

plugin-repos/starlark-apps/frame_extractor.py (3)

47-102: Consider using context manager to ensure file handle cleanup.

Image.open() returns a file-like object that holds a file handle open. While PIL often handles this gracefully, explicitly closing the image or using a context manager ensures proper resource cleanup, which is important for Raspberry Pi's limited resources.

♻️ Suggested improvement

     def load_webp(self, webp_path: str) -> Tuple[bool, Optional[List[Tuple[Image.Image, int]]], Optional[str]]:
         ...
         try:
-            img = Image.open(webp_path)
-
-            # Check if animated
-            is_animated = getattr(img, "is_animated", False)
-
-            if not is_animated:
-                # Static image - single frame
-                logger.debug(f"Loaded static WebP: {webp_path}")
-                return True, [(img.copy(), self.default_frame_delay)], None
-
-            # Animated WebP - extract all frames
-            frames = []
-            frame_count = getattr(img, "n_frames", 1)
-            ...
+            with Image.open(webp_path) as img:
+                # Check if animated
+                is_animated = getattr(img, "is_animated", False)
+
+                if not is_animated:
+                    # Static image - single frame
+                    logger.debug(f"Loaded static WebP: {webp_path}")
+                    return True, [(img.copy(), self.default_frame_delay)], None
+
+                # Animated WebP - extract all frames
+                frames = []
+                frame_count = getattr(img, "n_frames", 1)
+                ...

As per coding guidelines, cleaning up resources regularly is important for Raspberry Pi's limited RAM.

---

95-102: Use logger.exception() for error logging with stack traces.

When logging errors in exception handlers, logger.exception() automatically includes the stack trace, which aids remote debugging on Raspberry Pi.

♻️ Suggested fix

         except FileNotFoundError:
             error = f"WebP file not found: {webp_path}"
-            logger.error(error)
+            logger.exception(error)
             return False, None, error
         except Exception as e:
             error = f"Error loading WebP: {e}"
-            logger.error(error)
+            logger.exception(error)
             return False, None, error

As per coding guidelines, comprehensive logging is needed for remote debugging on Raspberry Pi.

---

280-284: Use logger.exception() for GIF conversion errors.

Similar to the earlier suggestion, using logger.exception() here will capture the stack trace for debugging.

         except Exception as e:
-            logger.error(f"Error converting frames to GIF: {e}")
+            logger.exception(f"Error converting frames to GIF: {e}")
             return None

web_interface/templates/v3/partials/starlark_apps.html (1)

53-210: Add dialog semantics and accessible labels for modal controls.

These modals lack role="dialog", aria-modal, and an accessible label for icon-only close buttons. This hurts screen readers and keyboard navigation. Recommend adding ARIA attributes and button labels (apply to all three modals).

♿ Example accessibility additions (repeat for other modals)

-<div id="upload-star-modal" class="fixed inset-0 bg-black bg-opacity-50 hidden z-50 flex items-center justify-center">
+<div id="upload-star-modal" class="fixed inset-0 bg-black bg-opacity-50 hidden z-50 flex items-center justify-center"
+     role="dialog" aria-modal="true" aria-labelledby="upload-star-title">
@@
-    <div class="p-6">
+    <div class="p-6">
         <div class="flex items-center justify-between mb-4">
-            <h3 class="text-xl font-bold text-gray-900">Upload Starlark App</h3>
-            <button onclick="closeUploadModal()" class="text-gray-500 hover:text-gray-700">
+            <h3 id="upload-star-title" class="text-xl font-bold text-gray-900">Upload Starlark App</h3>
+            <button onclick="closeUploadModal()" class="text-gray-500 hover:text-gray-700" aria-label="Close upload modal">
                 <i class="fas fa-times text-xl"></i>
             </button>
         </div>

plugin-repos/starlark-apps/config_schema.json (1)

76-96: Disallow unknown keys inside transition to keep validation strict.

The root schema blocks extra properties, but transition allows any unknown fields. Tighten this to avoid silent typos.

♻️ Proposed schema tightening

     "transition": {
       "type": "object",
       "description": "Transition settings for app display",
       "properties": {
@@
         "enabled": {
           "type": "boolean",
           "default": true
         }
-      }
+      },
+      "additionalProperties": false
     }

plugin-repos/starlark-apps/tronbyte_repository.py (1)

81-89: Prefer specific exceptions and stack traces in logs.
Bare Exception hides root causes and logger.error drops stack traces. Catch specific exceptions (e.g., ValueError, requests.RequestException, OSError) and use logger.exception with a consistent context prefix for troubleshooting. As per coding guidelines, use structured logging and avoid bare exceptions.

♻️ Example pattern (apply similarly in the other blocks)

-        except requests.Timeout:
-            logger.error(f"Request timeout: {url}")
+        except requests.Timeout:
+            logger.exception("[Starlark Repo] Request timeout: %s", url)
             return None
-        except requests.RequestException as e:
-            logger.error(f"Request error: {e}")
+        except requests.RequestException:
+            logger.exception("[Starlark Repo] Request error: %s", url)
             return None
-        except Exception as e:
-            logger.error(f"Unexpected error: {e}")
+        except ValueError:
+            logger.exception("[Starlark Repo] Invalid JSON response: %s", url)
             return None

Also applies to: 112-114, 251-252

web_interface/blueprints/api_v3.py (1)

6260-6274: Validate Starlark app config against schema before saving.

app.config.update(data) persists any keys/types and immediately re-renders. If the payload doesn’t match app.schema, the app can be left in a broken state. Consider validating (and rejecting) or filtering against app.schema before saving and rendering.

plugin-repos/starlark-apps/manager.py (2)

164-165: Prefer display_manager.width/height for size queries.

The plugin currently reads display_manager.matrix.*. Using display_manager.width/height (with fallback) keeps compatibility with other display managers and aligns with project guidance.

🔧 Suggested access pattern

-            display_width = self.display_manager.matrix.width
-            display_height = self.display_manager.matrix.height
+            display_width = getattr(self.display_manager, "width", self.display_manager.matrix.width)
+            display_height = getattr(self.display_manager, "height", self.display_manager.matrix.height)

-                width = self.display_manager.matrix.width
-                height = self.display_manager.matrix.height
+                width = getattr(self.display_manager, "width", self.display_manager.matrix.width)
+                height = getattr(self.display_manager, "height", self.display_manager.matrix.height)

Based on learnings, use display_manager.width/height to support different display sizes.

Also applies to: 462-464

---

412-420: Consider using cache file mtime for TTL across restarts.

app.last_render_time resets on restart, so cached renders are ignored even if still fresh. Basing TTL on cache_file mtime keeps the cache effective across restarts.

🧹 Optional TTL tweak

-            if (not force and use_cache and app.cache_file.exists() and
-                (current_time - app.last_render_time) < cache_ttl):
-                # Use cached render
-                self.logger.debug(f"Using cached render for: {app.app_id}")
-                return self._load_frames_from_cache(app)
+            if not force and use_cache and app.cache_file.exists():
+                cache_age = current_time - app.cache_file.stat().st_mtime
+                if cache_age < cache_ttl:
+                    # Use cached render
+                    self.logger.debug(f"Using cached render for: {app.app_id}")
+                    return self._load_frames_from_cache(app)

scripts/download_pixlet.sh (5)

31-36: Windows architecture mapping is defined but never used.

The download_binary function handles Windows binaries (lines 44-46), but the ARCHITECTURES array does not include any Windows entries. Either add Windows architecture or remove the dead Windows-handling code.

♻️ Option A: Add Windows support

 declare -A ARCHITECTURES=(
     ["linux-amd64"]="pixlet_Linux_x86_64.tar.gz"
     ["linux-arm64"]="pixlet_Linux_arm64.tar.gz"
     ["darwin-amd64"]="pixlet_Darwin_x86_64.tar.gz"
     ["darwin-arm64"]="pixlet_Darwin_arm64.tar.gz"
+    ["windows-amd64"]="pixlet_Windows_x86_64.zip"
 )

Note: Windows archives are typically .zip, not .tar.gz, which would require additional extraction logic.

♻️ Option B: Remove unused Windows handling

-    # Add .exe for Windows
-    if [[ "$arch" == *"windows"* ]]; then
-        binary_name="${binary_name}.exe"
-    fi

---

62-63: Declare and assign separately to avoid masking return values.

If mktemp -d fails, the error is masked by the local declaration. Split declaration and assignment for safer error handling under set -e.

♻️ Proposed fix

-    local temp_dir=$(mktemp -d)
-    local temp_file="$temp_dir/$archive_name"
+    local temp_dir
+    temp_dir=$(mktemp -d)
+    local temp_file="$temp_dir/$archive_name"

---

80-80: Declare and assign separately for extracted_binary.

Same issue as above—if find fails, the return value is masked.

♻️ Proposed fix

-    local extracted_binary=$(find "$temp_dir" -name "pixlet" -o -name "pixlet.exe" | head -n 1)
+    local extracted_binary
+    extracted_binary=$(find "$temp_dir" -name "pixlet" -o -name "pixlet.exe" | head -n 1)

---

100-101: Declare size separately and handle potential stat failure gracefully.

The chained stat command with || fallback works, but the local declaration still masks the overall exit status.

♻️ Proposed fix

-    local size=$(stat -f%z "$output_path" 2>/dev/null || stat -c%s "$output_path" 2>/dev/null)
+    local size
+    size=$(stat -f%z "$output_path" 2>/dev/null || stat -c%s "$output_path" 2>/dev/null || echo "unknown")

---

121-125: Avoid ls | grep for listing files.

Parsing ls output is fragile. Use a glob or find instead for robustness with unusual filenames.

♻️ Proposed fix

 echo ""
 echo "Installed binaries:"
-ls -lh "$BIN_DIR" | grep -v "^total" || echo "No binaries found"
+if compgen -G "$BIN_DIR/*" > /dev/null 2>&1; then
+    ls -lh "$BIN_DIR"/*
+else
+    echo "No binaries found"
+fi

plugin-repos/starlark-apps/pixlet_renderer.py (2)

142-157: Consider adding logging for edge cases in _get_safe_working_directory.

When the method returns None due to an exception or empty path, no logging occurs. This could make debugging difficult on Raspberry Pi deployments.

♻️ Proposed enhancement

     def _get_safe_working_directory(self, star_file: str) -> Optional[str]:
         """
         Get a safe working directory for subprocess execution.
 
         Args:
             star_file: Path to .star file
 
         Returns:
             Resolved parent directory, or None if empty or invalid
         """
         try:
             resolved_parent = os.path.dirname(os.path.abspath(star_file))
             # Return None if empty string to avoid FileNotFoundError
-            return resolved_parent if resolved_parent else None
+            if not resolved_parent:
+                logger.debug(f"Empty parent directory for star_file: {star_file}")
+                return None
+            return resolved_parent
         except (OSError, ValueError):
+            logger.debug(f"Could not resolve working directory for: {star_file}")
             return None

---

320-331: Move import json to module level.

While importing inside the function reduces initial load time, json is a standard library module with negligible import cost. Module-level imports improve code clarity and are the conventional Python style.

♻️ Proposed fix

At line 14, add json to imports:

 from typing import Dict, Any, Optional, Tuple
+import json

Then at line 322:

-                import json
                 try:
                     schema = json.loads(result.stdout)

plugin-repos/starlark-apps/tronbyte_repository.py (2)

91-91: Use explicit Optional[str] type hint instead of implicit None default.

PEP 484 prohibits implicit Optional. The type hint should explicitly indicate the parameter can be None.

♻️ Proposed fix

-    def _fetch_raw_file(self, file_path: str, branch: str = None) -> Optional[str]:
+    def _fetch_raw_file(self, file_path: str, branch: Optional[str] = None) -> Optional[str]:

---

252-262: Catch specific exceptions for file operations instead of broad Exception.

Per coding guidelines, catch specific exceptions. File writing can raise OSError (includes IOError, PermissionError, etc.).

♻️ Proposed fix

         try:
             output_path.parent.mkdir(parents=True, exist_ok=True)
             with open(output_path, 'w', encoding='utf-8') as f:
                 f.write(content)

             logger.info(f"Downloaded {app_id}.star to {output_path}")
             return True, None

-        except Exception as e:
-            logger.error(f"Failed to save .star file: {e}")
+        except OSError as e:
+            logger.exception(f"Failed to save .star file: {e}")
             return False, f"Failed to save file: {e}"

web_interface/blueprints/api_v3.py (1)

6515-6522: Clamp repository browse limit to a sane max.
A huge limit can amplify API latency and memory usage. Consider bounding to a reasonable range.

♻️ Suggested change

-        limit = request.args.get('limit', 50, type=int)
+        limit = request.args.get('limit', 50, type=int)
+        if limit is None:
+            limit = 50
+        limit = max(1, min(limit, 200))

plugin-repos/starlark-apps/manager.py (3)

76-84: Use logger.exception for better debugging on Raspberry Pi.

When logging errors with exception context, logger.exception automatically includes the stack trace, which is valuable for remote debugging on Pi deployments. As per coding guidelines, comprehensive logging aids remote troubleshooting.

♻️ Proposed fix

     def save_config(self) -> bool:
         """Save current configuration to file."""
         try:
             with open(self.config_file, 'w') as f:
                 json.dump(self.config, f, indent=2)
+            return True
         except Exception as e:
-            logger.error(f"Could not save config for {self.app_id}: {e}")
+            logger.exception(f"Could not save config for {self.app_id}: {e}")
             return False
-        return True

---

305-311: Use logger.exception for full stack trace in error handler.

♻️ Proposed fix

         except Exception as e:
-            self.logger.error(f"Error getting magnify recommendation: {e}")
+            self.logger.exception(f"Error getting magnify recommendation: {e}")
             return {
                 'display_size': 'unknown',
                 'calculated_magnify': 1,
                 'recommendations': []
             }

---

443-454: Use logger.exception for full stack traces in error handlers.

Multiple error handlers here use logger.error but would benefit from logger.exception for better remote debugging on Raspberry Pi.

♻️ Proposed fix

                 try:
                     # Use safe_app_id for internal storage to match directory structure
                     app = StarlarkApp(safe_app_id, app_dir, app_manifest)
                     self.apps[safe_app_id] = app
                     self.logger.debug(f"Loaded app: {app_id} (sanitized: {safe_app_id})")
                 except Exception as e:
-                    self.logger.error(f"Error loading app {app_id}: {e}")
+                    self.logger.exception(f"Error loading app {app_id}: {e}")

             self.logger.info(f"Loaded {len(self.apps)} Starlark apps")

         except Exception as e:
-            self.logger.error(f"Error loading apps manifest: {e}")
+            self.logger.exception(f"Error loading apps manifest: {e}")

web_interface/blueprints/api_v3.py (1)

6110-6138: Replace md5 with sha256 for the fallback app_id slug.

The repository's security linting rules (Ruff with Bandit enabled) will flag md5 usage. Swapping to sha256 eliminates the policy noise without changing behavior since you already slice to [:12].

🔁 Suggested change

-        hash_slug = hashlib.md5(app_id.encode()).hexdigest()[:12]
+        hash_slug = hashlib.sha256(app_id.encode()).hexdigest()[:12]

plugin-repos/starlark-apps/tronbyte_repository.py (2)

91-114: Use requests.RequestException + logger.exception here too.
Aligns with the session usage and improves diagnostics.

♻️ Suggested update

-        except Exception as e:
-            logger.error(f"Error fetching raw file {file_path}: {e}")
-            return None
+        except requests.RequestException as e:
+            logger.exception(f"Error fetching raw file {file_path}: {e}")
+            return None

---

274-279: Treat empty directories as a valid response.
if not data treats [] as failure; use an explicit None check.

🐛 Suggested fix

-        data = self._make_request(url)
-        if not data:
+        data = self._make_request(url)
+        if data is None:
             return False, None, "Failed to fetch app files"

web_interface/blueprints/api_v3.py (1)

6374-6384: Add the plugin_manager guard for consistency.

Most Starlark endpoints return a clear error if plugin_manager isn’t initialized. This endpoint skips that guard and can throw a 500 during early startup.

🔧 Guard for startup safety

 def get_starlark_app_config(app_id):
     """Get configuration for a Starlark app."""
     try:
+        if not api_v3.plugin_manager:
+            return jsonify({
+                'status': 'error',
+                'message': 'Plugin manager not initialized',
+                'pixlet_available': False
+            }), 500
+
         starlark_plugin = api_v3.plugin_manager.get_plugin('starlark-apps')

plugin-repos/starlark-apps/tronbyte_repository.py (1)

54-89: Prefer specific request/JSON exceptions and logger.exception.
This avoids swallowing unexpected errors and captures JSON decode failures cleanly.

♻️ Suggested update

-        except requests.Timeout:
-            logger.error(f"Request timeout: {url}")
-            return None
-        except requests.RequestException as e:
-            logger.error(f"Request error: {e}")
-            return None
-        except Exception as e:
-            logger.error(f"Unexpected error: {e}")
-            return None
+        except requests.Timeout:
+            logger.exception(f"Request timeout: {url}")
+            return None
+        except (requests.RequestException, ValueError) as e:
+            logger.exception(f"Request error: {e}")
+            return None

plugin-repos/starlark-apps/frame_extractor.py (1)

34-103: Prefer specific PIL exceptions and logger.exception.
This keeps unexpected failures visible without over-catching.

♻️ Suggested update

-import logging
-from typing import List, Tuple, Optional
-from PIL import Image
+import logging
+from typing import List, Tuple, Optional
+from PIL import Image, UnidentifiedImageError
...
-        except Exception as e:
-            error = f"Error loading WebP: {e}"
-            logger.error(error)
-            return False, None, error
+        except (UnidentifiedImageError, OSError) as e:
+            error = f"Error loading WebP: {e}"
+            logger.exception(error)
+            return False, None, error

plugin-repos/starlark-apps/manager.py (1)

226-233: Validate max_frames in config to avoid runtime errors.

_load_frames_from_cache forwards max_frames into optimize_frames; non‑ints will throw. Add validation alongside other config checks. As per coding guidelines, validate inputs and handle errors early.

🐛 Proposed fix

         # Validate default_frame_delay
         if "default_frame_delay" in self.config:
             delay = self.config["default_frame_delay"]
             if not isinstance(delay, (int, float)) or delay < 16 or delay > 1000:
                 self.logger.error("default_frame_delay must be a number between 16 and 1000")
                 return False
+
+        # Validate max_frames
+        if "max_frames" in self.config:
+            max_frames = self.config["max_frames"]
+            if max_frames is not None and (not isinstance(max_frames, int) or max_frames < 0):
+                self.logger.error("max_frames must be a non-negative integer or null")
+                return False

web_interface/blueprints/api_v3.py (3)

6151-6156: Preserve hyphens in app_id to avoid repository mismatches.

Current sanitization rewrites - to _, which can alter valid Tronbyte IDs and break lookups or cause collisions. Allow hyphens and trim _- at the ends.

♻️ Suggested adjustment

-    sanitized = re.sub(r'[^a-z0-9_]', '_', normalized)
+    sanitized = re.sub(r'[^a-z0-9_-]', '_', normalized)
@@
-    sanitized = sanitized.strip('_')
+    sanitized = sanitized.strip('_-')

---

6253-6256: Use Path.stem to avoid C:\fakepath\... rejection when deriving app_id.

Some browsers include client-side paths in file.filename, which then fails your path-separator guard. Use Path(...).stem to normalize.

♻️ Safer filename base

-        filename_base = file.filename.replace('.star', '') if file.filename else None
+        filename_base = Path(file.filename).stem if file.filename else None

---

6374-6380: Add a plugin_manager guard in get_starlark_app_config.

This endpoint calls api_v3.plugin_manager.get_plugin(...) without checking initialization, which can surface a 500 during early startup.

✅ Suggested guard

 def get_starlark_app_config(app_id):
     """Get configuration for a Starlark app."""
     try:
+        if not api_v3.plugin_manager:
+            return jsonify({
+                'status': 'error',
+                'message': 'Plugin manager not initialized',
+                'pixlet_available': False
+            }), 500
+
         starlark_plugin = api_v3.plugin_manager.get_plugin('starlark-apps')

web_interface/blueprints/pages_v3.py (1)

311-316: Catch specific template exceptions and avoid leaking internal errors.

This broad Exception handler was flagged in a previous review. The raw error string exposes internal details to clients. Use specific Jinja2 exceptions (TemplateNotFound, TemplateError, TemplateSyntaxError), add logging for remote debugging, and return a user-friendly message.

web_interface/static/v3/js/starlark_apps.js (2)

48-62: Init guard may skip HTMX-loaded UI elements.

The global window.starlarkAppsInitialized flag is set even if UI elements don't exist yet. If the Starlark partial is loaded via HTMX after the script initializes, setupEventListeners() won't re-run and buttons like upload-star-btn will lack handlers. Gate initialization on a stable element's existence, or make setupEventListeners() idempotent.

---

497-499: Preserve falsy config values (false, 0, "").

The || operator overwrites valid falsy values. If config[key] is false or 0, the form will incorrectly display field.default instead. Use nullish coalescing or explicit checks.

🛠️ Suggested fix

-            const value = config[key] || field.default || '';
+            const value = (config[key] !== undefined && config[key] !== null)
+                ? config[key]
+                : (field.default ?? '');

web_interface/static/v3/js/starlark_apps.js (1)

219-227: Consider checking response.ok before parsing JSON.

The code parses response.json() without checking HTTP status. If the server returns a non-JSON error (e.g., 502 from a proxy), parsing will throw. While the API appears to return JSON consistently, adding a status check improves resilience.

♻️ Optional improvement

             const response = await fetch('/api/v3/starlark/apps');
+            if (!response.ok) {
+                throw new Error(`HTTP ${response.status}`);
+            }
             const data = await response.json();

web_interface/blueprints/api_v3.py (3)

6156-6202: Preserve hyphens in app IDs to avoid repo mismatches.

Sanitizing - to _ can change repository IDs (e.g., foo-bar → foo_bar) and break lookups or create collisions.

🛠️ Suggested adjustment

-    sanitized = re.sub(r'[^a-z0-9_]', '_', normalized)
+    sanitized = re.sub(r'[^a-z0-9_-]', '_', normalized)
@@
-    sanitized = sanitized.strip('_')
+    sanitized = sanitized.strip('_-')

---

6241-6367: Strip client paths when deriving app_id from filename.

Browsers can submit C:\fakepath\...; using Path(...).stem avoids path separators causing unnecessary validation failures.

✅ Safer filename base

-        filename_base = file.filename.replace('.star', '') if file.filename else None
+        filename_base = Path(file.filename).stem if file.filename else None

---

6407-6435: Guard against uninitialized plugin_manager.

This endpoint calls api_v3.plugin_manager.get_plugin(...) without checking for None, which can throw during startup. Add a guard consistent with the other Starlark routes.

🛡️ Suggested guard

     try:
+        if not api_v3.plugin_manager:
+            return jsonify({
+                'status': 'error',
+                'message': 'Plugin manager not initialized',
+                'pixlet_available': False
+            }), 500
+
         starlark_plugin = api_v3.plugin_manager.get_plugin('starlark-apps')

web_interface/templates/v3/partials/starlark_apps.html (1)

8-10: Consider announcing Pixlet status updates to assistive tech.
This banner is JS-updated; adding role="status" and aria-live="polite" helps screen readers.

♻️ Suggested tweak

-    <div id="pixlet-status-banner" class="mb-6 p-4 rounded-lg border">
+    <div id="pixlet-status-banner" class="mb-6 p-4 rounded-lg border" role="status" aria-live="polite">